-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(s3-deployment): added property outputObjectKeys for BucketDeployment #31452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a few minor comments. Just want to verify the default behaviour and whether or not we should test when objectOutputKeys: true
is set as well.
Template.fromStack(stack).hasResourceProperties('Custom::CDKBucketDeployment', { | ||
OutputObjectKeys: false, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also verify that the previous behaviour still exists as well? That is, have a unit test to check that when OutputObjectKeys
is true, the object keys are in the output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May also be worth having a test to verify the default value of the attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, check integration test cases: packages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-big-response.ts && packages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! Just one check regarding one of the integ tests.
...ages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Awesome work, thanks!!
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #28579
Reason for this change
The CR lambda is essentially sending back the same data in the response which is hitting the limit for close to 50 object uploads.
Particularly this is being a limitation when using servicecatalog.ProductStack, if there are local assets beyond a particular number, the Custom::CDKBucketDeployment would fail with the error Response object is too long which is a hard limit of 4096 bytes.
Description of changes
outputObjectKeys
has been set to false by default for the service catalog product so that the error does not occur.Description of how you validated changes
Validated using a sample stack with the property set and confirmed the behavior. Also, the existing deployments would be unaffected.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license